[RFC 0002] Flux OCI support for Helm #690
Conversation
stefanprodan
added
enhancement
|
This is still on-going, and we will continue refining most of the code. We want to have early feedback so we can address any red flag. I would like to draw your attention on the fact that the |
|
I am still frowning when I see |
souleb
force-pushed
the
rfc-0002-helm-oci
branch
3 times, most recently
from
c921cbd
to
21bdf70
Compare
souleb
force-pushed
the
rfc-0002-helm-oci
branch
2 times, most recently
from
a81acae
to
eea3c22
Compare
|
I would still like @darkowlzz to do a review, even post-merge. |
| Get(name, version string) (*repo.ChartVersion, error) | ||
| // GetChartVersion returns a chart.ChartVersion from the remote repository. | ||
| DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error) | ||
| } |
There was a problem hiding this comment.
This interface represents a Repository, maybe it doesn't matter if it's remote or local, download could just be a copy for local repositories. Being in chart package, it could be confused with a remote chart due to calling it just Remote.
Based on the implementations of this, it seems to fit better in the internal/helm/repository package with a generic name like just Repository.
Also, the method descriptions seem to be outdated with some previous names.
We may also need to add an Unload() method to this interface. The various implementation can implement it in their own appropriate ways, no-op for some and GC signal for others. I believe this would improve some usage of this interface below for repository download. downloadFromOCIRepository() and downloadFromRepository() could be unified by moving StrategicallyLoadIndex() for non-OCI repository into the implementation of Get(). The existing Get() can be renamed to something else to not remove the current behavior option if needed. Or the Get() interface method could be renamed to GetChart() which would be more appropriate as the interface is a repository and GetChart() describes exactly what's being fetched, and aligns with DownloadChart().
There was a problem hiding this comment.
Remote and local chartBuilder have quite different code base, and a Repository in our code always download from a remote url.
I created the interface here because it is the only place where we need this abstraction for now.
I agree that the name needs to be changed and will propose a new one in the follow-up pr. I will also update the method description, thanks.
I agree to renaming Get to GetChart, that makes perfect sense. I'm not quite sure about having the Get method do more that what it does.
About the Unload() method in the interface, my feeling is that we should stick to having the minimum set of methods in the interface, instead of having no-op implementations.
| @@ -171,6 +152,121 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o | |||
| return result, nil | |||
| } | |||
|
|
|||
| func (b *remoteChartBuilder) downloadFromOCIRepository(remote *repository.OCIChartRepository, remoteRef RemoteReference, buildResult *Build, opts BuildOptions) (*bytes.Buffer, error) { | |||
There was a problem hiding this comment.
The function arguments can be simplified by removing remote repository to be passed, as remoteChartBuilder embeds the remote. If the interface is expanded as suggested above, Get(), Unload() and DownloadChart() can be directly called on the remote without the need of any type casting.
If the remote interface is not expanded to have Unload(), as this function is aware of when it's called, instead of passing a typed remote, it can internally do the type casting. I think expanding the remote interface would help not have two separate functions for downloading, as most of the code is exactly the same.
Also, the buildResult argument can just become a returned value. Don't see any specific reason to accept it as a pointer. Whenever there's an error and buildResult is not assigned, the caller always ends further building and stops any usage of buildResult.
All these comments apply to downloadFromRepository() as well.
There was a problem hiding this comment.
Now that I read this, an Unload() method in the interface makes more sense 👍
|
|
||
| // Registry config | ||
| config := &configuration.Configuration{} | ||
| port, err := freeport.GetFreePort() |
There was a problem hiding this comment.
Maybe it's not worth importing a new dependency just to get a free port.
We have just done it simply in some tests like
source-controller/pkg/git/strategy/proxy/strategy_proxy_test.go
Lines 60 to 62 in 841ed7a
There was a problem hiding this comment.
see #728
Edit:
This causes error on the e2e tests. keeping the current code.
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
HelmRepository_OCI reconcilers to make to retry on failure The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
| func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error { | ||
| if !obj.DeletionTimestamp.IsZero() { | ||
| if !obj.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != sourcev1.HelmRepositoryTypeDefault) { |
There was a problem hiding this comment.
How about introducing a method on HelmRepositoryReconciler, say isDifferentType(obj), which does this non empty and not default type check? I believe that'd make it more readable and less error prone when writing the same conditional check multiple times, here and above in Reconcile().
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
The setupRegistryServer has been refactored to take into account fluxcd#690 reviews. Signed-off-by: Soule BA <[email protected]>
|
Hi, do you know when this feature will be released? |
|
@sandrotaje this was released a few days ago on version v0.25.0. Since then there were some minor fixes, so please use the latest version of the source-controller. Here's the official documentation on how to use it: https://fluxcd.io/docs/components/source/helmrepositories/#helm-oci-repository |
|
Hi, Does that means we can now use helm repositories on Azure Container Registry? This document says using Charts from OCI Registries is not yet supported. |
|
@jobinjosem1 we shall update the doc. |
This is an implementation of RFC 0002.
Closes #669
If implemented:
users will be able to declare OCI HelmRepository by using
the
.Spec.Typefield of the HelmRepository API. Contrary to the HTTP/S HelmRepository no index.yaml is reconciled from source, instead a simple url and credentials validation is performed.users will be able to declare the new OCI HelmRepository type as source using the
.Spec.SourceReffield of the HelmChart API. This will result in reconciling a chart from an OCI repository.TODOS:
not covered by this pull request
Tests
deployed on AWS EKS